-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Replaces some of the CL_* enums with PI_* enums. #1239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@rbegam, please, resolve merge conflict and clang-format check issues. |
a9ef255
to
5a14600
Compare
@rbegam, please, resolve merge conflicts. |
Signed-off-by: rbegam <[email protected]>
Signed-off-by: Rehana Begam <[email protected]>
5a14600
to
107c86d
Compare
@bader done. please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a few nits.
Signed-off-by: Rehana Begam <[email protected]>
Signed-off-by: Rehana Begam <[email protected]>
@romanovvlad all comments have been resolved. please submit as I do not have write permission. thanks. |
PI_COMMAND_EVENTS_WAIT = CL_COMMAND_MARKER, | ||
PI_COMMAND_MEMBUFFER_COPY = CL_COMMAND_COPY_BUFFER, | ||
PI_COMMAND_MEMBUFFER_FILL = CL_COMMAND_FILL_BUFFER | ||
} _pi_command_type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes break the CUDA plugin, e.g.
llvm/sycl/plugins/cuda/pi_cuda.cpp
Line 1634 in dc729a7
_pi_event::make_native(PI_COMMAND_MEMBUFFER_WRITE, command_queue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are renamed i.e., PI_COMMAND_MEMBUFFER_WRITE -> PI_COMMAND_TYPE_MEM_BUFFER_WRITE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the renaming was not propagated to the CUDA plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no buildbot to check that at the moment, but we are working on adding it.
Meanwhile to check locally you can try this https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md#build-dpc-toolchain-with-support-for-nvidia-cuda
The CUDA backend cannot currently build due to the renaming of descriptors done in #1239. This PR adjusts the backend to the new descriptor names. Signed-off-by: Steffen Larsen <[email protected]>
We made a mistake several years ago by introducing `pi_mem_advice` argument into a public interface in intel#1239. This mistake was caught a year later and addressed in intel#4100, but unfortunately instead of removing this incorrect overload, it was deprecated. Even deprectation was done improperly, because this method is *not* deprecated in SYCL 2020 - it *doesn't exists* in SYCL 2020. This PR removes the method completely, even though our messaging was wrong. The assumption is that there are no actual users who went and used undocumented non-public `pi_mem_advice` in their codebases.
We made a mistake several years ago by introducing `pi_mem_advice` argument into a public interface in #1239. This mistake was caught a year later and addressed in #4100, but unfortunately instead of removing this incorrect overload, it was only deprecated. The deprecation wasn't done quite correctly, because this method is *not* deprecated in SYCL 2020 - it *doesn't exists* in SYCL 2020. This PR removes the method completely, even though our messaging was wrong. The assumption is that there are no actual users who went and used undocumented non-public `pi_mem_advice` in their codebases. This is patch is essentially a by-product of #14145 and it is done to simplify that change, i.e. PI plugins removal should not be ABI-breaking by itself, we just need to cleanup some of our exported symbols.
Signed-off-by: rbegam [email protected]
Co-Authored-By: Alexey Bader [email protected]